-
-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make initialization of tokenizer and detokenizer optional #3748
Conversation
69e732d
to
50cc4b7
Compare
@GeauxEric Thank you for the contribution! Could you update this branch with the changes merged in the main branch? Happy to review after the conflicts are resolved! |
9c6d6cc
to
3a6a0e7
Compare
with a ',', it becomes a tuple, and deemed as True.
3a6a0e7
to
676256f
Compare
@ywang96 |
Maybe others can help take a look? @simon-mo? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review - I can see the purpose of having this. Since it doesn't affect other downstream components too much, we should be fine with having this feature!
vllm/engine/arg_utils.py
Outdated
@@ -95,6 +96,10 @@ def add_cli_args( | |||
type=str, | |||
default=EngineArgs.tokenizer, | |||
help='name or path of the huggingface tokenizer to use') | |||
parser.add_argument( | |||
'--skip_tokenizer_init', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format nit
'--skip_tokenizer_init', | |
'--skip-tokenizer-init', |
eos_token_id = None | ||
if self.tokenizer: | ||
eos_token_id = self.tokenizer.get_lora_tokenizer( | ||
lora_request).eos_token_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it's worth having an warning here about eos_token_id
being None
.
Could you please take another look and consider merging/approving it when you have a moment? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeauxEric Sorry for the delayed review and thank you for addressing the comments and contributing this PR!
…ct#3748) Co-authored-by: Yun Ding <[email protected]> Co-authored-by: Roger Wang <[email protected]>
…ct#3748) Co-authored-by: Yun Ding <[email protected]> Co-authored-by: Roger Wang <[email protected]>
…ct#3748) Co-authored-by: Yun Ding <[email protected]> Co-authored-by: Roger Wang <[email protected]>
…ct#3748) Co-authored-by: Yun Ding <[email protected]> Co-authored-by: Roger Wang <[email protected]>
…ct#3748) Co-authored-by: Yun Ding <[email protected]> Co-authored-by: Roger Wang <[email protected]>
…ct#3748) Co-authored-by: Yun Ding <[email protected]> Co-authored-by: Roger Wang <[email protected]>
…ct#3748) Co-authored-by: Yun Ding <[email protected]> Co-authored-by: Roger Wang <[email protected]>
…ct#3748) Co-authored-by: Yun Ding <[email protected]> Co-authored-by: Roger Wang <[email protected]>
…ct#3748) Co-authored-by: Yun Ding <[email protected]> Co-authored-by: Roger Wang <[email protected]>
Add a flag to initialize the LLM engine to disable tokenizer.
If tokenizer is disabled, so does detokenizer.
FIX #3635
FIX #3647